fix: prevent false success for unsupported system backend - #65
Conversation
Signed-off-by: muhammadburhandevv-hub <muhammadburhandevv@gmail.com>
Same defect as #66, reached from a different direction — needs a rebaseThe problem you are fixing is real and it is the same one #66 addresses: Both PRs edit So I have approved #66 to land first. Verified locally: this branch on current master is 9 failed / 201 passed, and it conflicts with #66 in What is yours alone, and worth keepingTwo things in here are not in #66, and I would not want them lost in the rebase:
Suggested rebaseOnce #66 lands, drop your One question worth settling while you are in there: #66 standardised on |
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Approving for the config.py half, which is a real bug nobody else has covered.
The unique and valuable part
System-image settings are not a compilation backend. Keep them separate.
That is exactly right, and I verified the failure. config.py:230 sets
if raw.get("system") and isinstance(raw["system"], dict):
backend_config.update(raw["system"])
if backend == "auto":
backend = "system"but "system" is not a backend:
ALL_BACKENDS = {"cmake", "make", "meson", "cargo", "kbuild", "ninja"}So a build.yaml with a perfectly valid system: section selects a backend the
dispatcher has never heard of. Before #66 that fell through silently — the false
success in your title. With #66 merged it now fails, which is better, but with a
message that blames the user's own valid configuration:
$ ebuild build # build.yaml has a system: section
[error] Unknown build backend 'system'. Supported backends: cargo, cmake,
kbuild, make, meson.
exit=1
ebuild system exists as a command, so system-image builds are a supported
feature being reported as an unknown backend. Your split — keeping
system_config out of backend selection entirely, with 'system' must be a mapping validation and a None guard — fixes the cause rather than the
symptom. Neither #66 nor any other open PR touches this.
The dispatch.py half overlaps #66
BackendError(RuntimeError) here and UnknownBackendError(ValueError, RuntimeError) in #66 solve the same problem. #66's inherits from both
deliberately, because the two existing test suites disagreed about which
exception to expect:
tests/ebuildexpectsValueError("Unknown build backend '<name>'")while
tests/unitexpectsRuntimeErrormatching "ninja". Both are legitimate
readings.
BackendError(RuntimeError) alone would fail the tests/ebuild expectation. #66
is also already approved and carries the master syntax repair (#87), so it
lands first either way.
Suggested path
Rebase onto #66 and drop the dispatch.py and tests/ebuild/test_dispatch.py
hunks, keeping config.py, tests/ebuild/test_config_validation.py and
tests/ebuild/test_build_cli.py. This then reviews cleanly on its own merits and
the part that is uniquely yours is not stuck behind a conflict.
Approving now so it is not blocked on me, but it does need that rebase before it
can merge.
|
Hi EmbeddedOS Team,
I’ve addressed the review feedback and updated the eBuild PR (#65). With
all three PRs rebased and ready (eBoot #57, eOS #74, and eBuild #65), I’d
love to check in on the next steps in the evaluation process.
Could we schedule a short call to discuss the role and next steps? Please
let me know what day and time work best for your team.
Best,
Muhammad
…On Mon, 31 Aug 2026 at 02:15, Srikanth Patchava ***@***.***> wrote:
***@***.**** approved this pull request.
Approving for the config.py half, which is a real bug nobody else has
covered.
The unique and valuable part
System-image settings are not a compilation backend. Keep them separate.
That is exactly right, and I verified the failure. config.py:230 sets
if raw.get("system") and isinstance(raw["system"], dict):
backend_config.update(raw["system"])
if backend == "auto":
backend = "system"
but "system" is not a backend:
ALL_BACKENDS = {"cmake", "make", "meson", "cargo", "kbuild", "ninja"}
So a build.yaml with a perfectly valid system: section selects a backend
the
dispatcher has never heard of. Before #66
<#66> that fell through
silently — the false
success in your title. With #66
<#66> merged it now fails,
which is better, but with a
message that blames the user's own valid configuration:
$ ebuild build # build.yaml has a system: section
[error] Unknown build backend 'system'. Supported backends: cargo, cmake,
kbuild, make, meson.
exit=1
ebuild system exists as a command, so system-image builds are a supported
feature being reported as an unknown backend. Your split — keeping
system_config out of backend selection entirely, with 'system' must be a
mapping validation and a None guard — fixes the cause rather than the
symptom. Neither #66 <#66>
nor any other open PR touches this.
The dispatch.py half overlaps #66
<#66>
BackendError(RuntimeError) here and UnknownBackendError(ValueError,
RuntimeError) in #66 <#66>
solve the same problem. #66
<#66>'s inherits from *both*
deliberately, because the two existing test suites disagreed about which
exception to expect:
tests/ebuild expects ValueError("Unknown build backend '<name>'") while
tests/unit expects RuntimeError matching "ninja". Both are legitimate
readings.
BackendError(RuntimeError) alone would fail the tests/ebuild expectation.
#66 <#66>
is also already approved and carries the master syntax repair (#87
<#87>), so it
lands first either way.
Suggested path
Rebase onto #66 <#66> and
drop the dispatch.py and tests/ebuild/test_dispatch.py
hunks, keeping config.py, tests/ebuild/test_config_validation.py and
tests/ebuild/test_build_cli.py. This then reviews cleanly on its own
merits and
the part that is uniquely yours is not stuck behind a conflict.
Approving now so it is not blocked on me, but it does need that rebase
before it
can merge.
—
Reply to this email directly, view it on GitHub
<#65?email_source=notifications&email_token=CF4LHR5YGDAIUAGGSAPQJJT5MSKODA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMBWGE4TCMJZGUYKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-5061911950>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/CF4LHRYGPPN5NN7BTI4WTLT5MSKODAVCNFSNUABGKJSXA33TNF2G64TZHMYTCOJQGEZDKNZYGE5US43TOVSTWNJSG43TQMJWGUZTJILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/CF4LHR6UOO6W5KKTZKOAYRD5MSKODA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMBWGE4TCMJZGUYKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/CF4LHR3LTFGEOL4GAAXACUD5MSKODA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMBWGE4TCMJZGUYKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
PackageRecipe.validate() accepts any non-empty version string, but the
registry ordered versions with
sorted(versions, key=lambda v: [int(x) for x in v.split('.')])
which raises ValueError for anything that is not dotted integers. Real
recipes are full of those: a leading v (littlefs and FreeRTOS both publish
their tags that way -- the recipe example in the book uses `tag: V10.5.1`),
pre-release tags like 3.6.0-rc1, and build metadata like 1.3.1+patch2.
The blast radius is wider than the odd package itself. The key was
duplicated across get(), list_packages() and list_all_versions(), and
* get(name) with no version scans every version of that package, so one
such recipe breaks that package entirely;
* list_packages() scans every package, and PackageResolver builds its
"package not found in registry. Available: ..." message from it -- so a
single unusual recipe anywhere in the registry turns an ordinary
missing-package error into a ValueError traceback.
Replace the three copies with one version_sort_key(). Ordering: a leading
v/V is ignored; all-digit components compare numerically so 1.10.0 still
sorts above 1.9.0; any other component compares as text and ranks below a
numeric one; a -/+ suffix ranks below the same version without one, so
3.6.0-rc1 < 3.6.0. The order is total and never raises, which is the
property that matters here -- one unusual recipe must not decide whether
lookup works for the packages around it.
Deliberately not a full PEP 440 / semver implementation. That would mean a
dependency or a lot more code for a comparison the recipe format does not
specify; this defines the rules it does need and documents them.
Tests: tests/ebuild/test_package_registry.py grows from 1 case to 12,
covering v-prefixes, pre-releases, build metadata, date-stamped and
non-numeric versions, one odd version among good ones, and a totality check
over the key. Nine of the eleven behavioural cases fail against the unfixed
registry.py (checked by running them against it); the other two are the
pre-existing numeric-order test and "2024.06", which parsed as ints before.
Docs: the ordering rules are now in docs/book/book.md section 12.5.
Note on CI: master currently fails for reasons unrelated to this change --
ebuild/build/dispatch.py has a duplicated `else:` from a merge, so the
module does not parse (PRs embeddedos-org#65/embeddedos-org#66 address it), and ninja_backend.py is
missing _object_path. This branch leaves the failure set exactly as it
found it: 26 failed / 162 passed before and after, identical lists.
Verified: pytest tests/ebuild/test_package_registry.py -> 12 passed.
Verified: ruff check --select=E,F,W --ignore=E501 and mypy
--ignore-missing-imports on both changed files -> clean.
|
Hi EmbeddedOS Team,
Hope you’re doing well!
I’m checking in regarding the updates made across the assessment PRs.
Now that the screening task revisions are complete, I’d appreciate clarity
on the next steps in your recruitment process. To ensure alignment, I want
to confirm that I am evaluating paid career opportunities and am not
available for volunteer work.
Could you share the expected timeline for moving into formal interviews, or
let me know when your recruiter might be available for a brief call?
Best,
Muhammad
…On Tue, 1 Sept 2026 at 15:06, Srikanth Patchava ***@***.***> wrote:
Merged #65 <#65> into master.
—
Reply to this email directly, view it on GitHub
<#65?email_source=notifications&email_token=CF4LHRYLPWWMYJ2QYVZKZA35M2NT7A5CNFSNUABQM5UWIORPF5TWS5BNNB2WEL2JONZXKZKFOZSW45CON52GSZTJMNQXI2LPNYXTGMBTGM3TQMBWGQ2TFJTSMVQXG33OUZQXK5DIN5ZKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#event-30337806452>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/CF4LHR32CKYWRWP4HNQIPQ35M2NT7AVCNFSNUABGKJSXA33TNF2G64TZHMYTCOJQGEZDKNZYGE5US43TOVSTWNJSG43TQMJWGUZTJILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/CF4LHR5AE3LWEAVH67JZ6A35M2NT7A5CNFSNUABQM5UWIORPF5TWS5BNNB2WEL2JONZXKZKFOZSW45CON52GSZTJMNQXI2LPNYXTGMBTGM3TQMBWGQ2TFJTSMVQXG33OUZQXK5DIN5ZKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/CF4LHR32BWBOREGTDD4EUWD5M2NT7A5CNFSNUABQM5UWIORPF5TWS5BNNB2WEL2JONZXKZKFOZSW45CON52GSZTJMNQXI2LPNYXTGMBTGM3TQMBWGQ2TFJTSMVQXG33OUZQXK5DIN5ZKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
2 participants
Summary
Fixes an issue where eBuild could report a successful build even though no
supported build backend ran. A
system:section is now kept separate frombackend configuration, and unsupported backends fail with a clear error.
Type of Change
Changes
system:settings separate from compilation backend configuration.BackendErrorwhen the external dispatcher cannot handle a backend.Testing
ctest --test-dir build --output-on-failure)Focused Python regression tests:
Additional validation:
git diff --checkpassed.198 passed, 11 failed, 1 skipped.Pre-Submission Checklist
-Wall -Wextra -Werrorfor C)<type>(<scope>): <description>conventionRelated Issues
No related issue number.
Screenshots / Logs
Additional Notes
systemcompilation backend.eBuild implementation and its pytest suite.
cpio, whichis unavailable in the Windows test environment. The remaining failures come
from an existing missing
NinjaBackend._object_pathmethod.